Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: auto-set block timestamp for historical queries #15448

Merged
merged 22 commits into from
Mar 28, 2023
Merged

feat: auto-set block timestamp for historical queries #15448

merged 22 commits into from
Mar 28, 2023

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Mar 17, 2023 •
edited
Loading

Description

Closes: #12226

This change automatically set's the block time on the Context that is provided to all queries. It does this by querying for the CommitInfo for that particular version/height from the RMS.

cc @ValarDragon @tac0turtle @julienrbrt

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the C:CLI label Mar 17, 2023
@ValarDragon
Copy link
Contributor

Should we in a subsequent PR maintain an in-memory cache for historic block times ? Otherwise the extra query seems fine to me!

@tac0turtle
Copy link
Member

i dont think we can reliably use blocks here as the pruning strategies between the two are out of sync fairly often

@ValarDragon
Copy link
Contributor

Ah I see, this implicitly assumed that the blocks pruning strategy would be more relaxed than the states pruning. If that doesn't hold, then a height-> timestamp map may have to be stored in state machine. (Which does make sense to do)

@alexanderbez
Copy link
Contributor Author

Gonna take another approach. Just hack into CommitInfo and store the timestamp there! No extra query needed, just one additional quick DB read.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot removed the C:CLI label Mar 21, 2023
@alexanderbez alexanderbez marked this pull request as ready for review March 22, 2023 16:54
@alexanderbez alexanderbez requested a review from a team as a code owner March 22, 2023 16:54
@github-prbot github-prbot requested a review from a team March 22, 2023 16:54
@alexanderbez alexanderbez enabled auto-merge (squash) March 27, 2023 13:46
@alexanderbez alexanderbez added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Mar 27, 2023
tac0turtle
tac0turtle approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this consensus breaking or is metadata outside the app hash

@facundomedica
Copy link
Member

is this consensus breaking or is metadata outside the app hash

As far as I understand the metadata is not included in the app hash. CommitInfo.Hash() uses only the hashes of the StoreInfos inside of it while the CommitInfo itself is written directly to the kv store.

Please confirm that's how it works @alexanderbez

@alexanderbez
Copy link
Contributor Author

Correct @facundomedica, this is not consensus breaking, only the resulting hash is used.

@alexanderbez alexanderbez merged commit ee9774a into main Mar 28, 2023
@alexanderbez alexanderbez deleted the bez/12226-auto-timestamp-query branch March 28, 2023 15:10
mergify bot pushed a commit that referenced this pull request Mar 28, 2023
@alexanderbez @mergify
feat: auto-set block timestamp for historical queries (#15448)
2084ca8 
(cherry picked from commit ee9774a)

# Conflicts:
#	CHANGELOG.md
#	api/cosmos/base/store/v1beta1/commit_info.pulsar.go
#	baseapp/abci.go
#	go.sum
#	simapp/go.mod
#	simapp/go.sum
#	store/go.mod
#	store/rootmulti/store.go
#	store/types/commit_info.pb.go
#	tests/go.mod
#	tests/go.sum
#	types/errors/errors.go
@mergify mergify bot mentioned this pull request Mar 28, 2023
Merged
@faddat faddat mentioned this pull request Mar 28, 2023
Closed
19 tasks
facundomedica pushed a commit that referenced this pull request Mar 28, 2023
@mergify @alexanderbez
feat: auto-set block timestamp for historical queries (backport #15448)…
b979f8b 
… (#15573)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
@ValarDragon
Copy link
Contributor

Awesome!!

@tac0turtle tac0turtle mentioned this pull request May 3, 2023
Closed
4 tasks
p0mvn pushed a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
@alexanderbez @p0mvn
feat: auto-set block timestamp for historical queries (cosmos#15448)
542b82a
@p0mvn p0mvn mentioned this pull request May 8, 2023
Merged
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
@p0mvn @alexanderbez
feat: auto-set block timestamp for historical queries (cosmos#15448) (#…
8a66b9a 
…440)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
@p0mvn
feat: auto-set block timestamp for historical queries (cosmos#15448) …
7dd4643 
…(backport #440)
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 8, 2023
@mergify @p0mvn
feat: auto-set block timestamp for historical queries (cosmos#15448) …
9310076 
…(backport #440) (#441)

* feat: auto-set block timestamp for historical queries (cosmos#15448) (backport #440)

* updates

* updates

* updates

---------

Co-authored-by: Roman <roman@osmosis.team>
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 9, 2023
@p0mvn
feat: auto-set block timestamp for historical queries (cosmos#15448) …
050283e 
…(backport #440)
p0mvn added a commit to osmosis-labs/cosmos-sdk that referenced this pull request May 9, 2023
@mergify @p0mvn
feat: auto-set block timestamp for historical queries (cosmos#15448) …
2afc03d 
…(backport #440) (#442)

* feat: auto-set block timestamp for historical queries (cosmos#15448) (backport #440)

* go mod tidy

---------

Co-authored-by: Roman <roman@osmosis.team>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@alexanderbez
feat: auto-set block timestamp for historical queries (cosmos#15448)
0033034
roy-dydx pushed a commit to dydxprotocol/cosmos-sdk that referenced this pull request Jul 11, 2023
@mergify @alexanderbez @roy-dydx
feat: auto-set block timestamp for historical queries (backport cosmo…
2b83cb7 
…s#15448) (cosmos#15573)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
@JoowonYun JoowonYun mentioned this pull request Dec 8, 2023
Merged
@JoowonYun JoowonYun mentioned this pull request Jan 12, 2024
Closed
1 task
@0xstepit 0xstepit mentioned this pull request Jul 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@facundomedica facundomedica facundomedica approved these changes

@testinginprod testinginprod Awaiting requested review from testinginprod testinginprod is a code owner

Assignees

@facundomedica facundomedica

@tac0turtle tac0turtle

Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:Store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect timestamp is used when querying at previous block heights
4 participants
@alexanderbez @ValarDragon @tac0turtle @facundomedica